Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add icon element class names for animation #7729

Conversation

lee-chase
Copy link
Member

Contributes to #7346

Currently, SVG Icon elements do not have classes to allow targetted CSS animation.

This PR adds the ability to utilise a className as part of the SVG metadata. It also adds a property to the Icon component to optionally add an automatically generated class based on the element type and index in the metadata.

NOTE 1: A slightly more robust auto generated class could be created, based on a selection of attribute values in the element.
NOTE 2: We could go a little further here and split Path elements on a 'z' and/or capitalized 'M' to make the animations more versatile. The same strategy used for differentiating auto generated classes could be used here.

Changelog

Changed

M packages/icon-build-helpers/src/builders/react/builder.js
M packages/icon-build-helpers/src/builders/react/components/Icon.js

Testing / Reviewing

Tried it out in a test app with and without prop 'addElementClasses' also with and without a className attribute set in metadata.json.

@lee-chase lee-chase requested a review from a team as a code owner February 3, 2021 15:33
@netlify
Copy link

netlify bot commented Feb 3, 2021

Deploy preview for carbon-elements ready!

Built with commit de479b1

https://deploy-preview-7729--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Feb 3, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit de479b1

https://deploy-preview-7729--carbon-components-react.netlify.app

@joshblack
Copy link
Contributor

@lee-chase thanks so much for putting this together! It's awesome to see the generation logic in there.

I'm a little hesitant about is adding this to the base packages, are there any other strategies we could use for enabling these kinds of transforms for adding classes to the paths?

One thing that came to mind is a dedicated package that uses the icon data from @carbon/icons and provides bespoke class names. It definitely isn't auto-generated, however, but may provide more control if that is needed.

@lee-chase
Copy link
Member Author

@joshblack Another option would be to provide a wrapper to the Icon component, or perhaps just an alternate that walks the children and creates a component with classes added on the fly. The performance hit would be tiny on small SVGs.

@lee-chase
Copy link
Member Author

Perhaps we can get together to discuss how #7751 / #7346 are progressed. Are there others you would suggest joining such a conversation @joshblack ?

@joshblack
Copy link
Contributor

@lee-chase I think moving forward @johnbister will be great on the design side.

On the technical side, I think it'd be great to have a dedicated package for this kind of stuff that uses the icon data from @carbon/icons to generate the icons with the preferred class names for the icons that are being worked on.

I think from that experiment we can get a feel for how widespread this use-case is and how folks will want to leverage it before folding it into @carbon/icons-react, if that makes sense.

Let me know what you think!

@lee-chase
Copy link
Member Author

@lee-chase I think moving forward @johnbister will be great on the design side.

On the technical side, I think it'd be great to have a dedicated package for this kind of stuff that uses the icon data from @carbon/icons to generate the icons with the preferred class names for the icons that are being worked on.

I think from that experiment we can get a feel for how widespread this use-case is and how folks will want to leverage it before folding it into @carbon/icons-react, if that makes sense.

Let me know what you think!

Will try and sort this at some point Josh, but struggling for time.

Base automatically changed from master to main March 8, 2021 16:35
@joshblack
Copy link
Contributor

Hey @lee-chase! 👋 Going to close this out since I think the group is going for a smaller approach first 👀 Let me know if this is not the case!

@joshblack joshblack closed this Apr 14, 2021
@github-actions
Copy link
Contributor

DCO Assistant Lite bot: Thanks for your submission! We ask that you sign our Developer Certificate of Origin before we can accept your contribution. You can sign the DCO by adding a comment below using this text:


I have read the DCO document and I hereby sign the DCO.


Lee Chase seems not to be a GitHub user. You need a GitHub account to be able to sign the DCO. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants